Code Review — PR #924: Fix overlay output-type picker

Overview

Fixes two independent bugs that made the overlay output-type picker unusable in 1.0 RC testing:

1. Root cause (hoisted popover stale content): WindowAnchoredPopoverEntry had identity-only Equatable. SwiftUI compared same-id entries as equal and skipped preference updates + re-renders, so the picker never navigated even though the model updated correctly.
2. First-click swallow: The non-focus-stealing overlay window consumed the first mouse-down as a window-activation event, making controls appear unclickable.

The solution replaces inline section expansion with an iPhone-style drill-down in a fixed frame, moves navigation state onto MapperViewModel (so it propagates across the hoisted window boundary), and adds FirstMouseHostingView plus a global outside-click monitor.

Correctness

Root cause fix is correct. Dropping Equatable from WindowAnchoredPopoverEntry and removing the cachedContent one-shot cache is the right call. The old comment acknowledged the AnyView diffing limitation but did not anticipate that caching would freeze navigation state. The new approach is simpler and correct.

Global monitor lifecycle looks right. removeDismissMonitors() is called on onDisappear, on entry-set collapse (ids empty), and before reinstall inside installDismissMonitors. No obvious leak path.

FirstMouseHostingView is minimal and safe. Scoped to the single class, mirrors what NativeDragNSView already does for the drag header.

AutoFocusTextField focus guard is correct. didFocus on the coordinator (not the view struct) persists across re-renders, so a re-render does not keep stealing focus from the user mid-type.

Issues & Suggestions

1. AnyView(contentBuilder()) called on every layout pass

In the new WindowAnchoredPopoverModifier, contentBuilder() is called inside the anchorPreference closure, which SwiftUI evaluates on every layout pass while the popover is open:

.anchorPreference(key: ..., value: .bounds) { anchor in
    guard isPresented else { return [] }
    return [WindowAnchoredPopoverEntry(
        ...
        content: AnyView(contentBuilder()),   // runs each layout pass
        ...
    )]
}

The old cachedContent was the cause of the bug, but also served as a layout-pass guard. The picker content is lightweight so this is not a crisis, but worth a comment acknowledging the trade-off. A middle ground if it ever becomes a problem: invalidate on a model-derived value (e.g. viewModel.outputPickerPage) rather than per-pass — but that is premature for now.

2. entries.map(\.id) allocated three times in WindowAnchoredPopoverHostModifier.body

.onChange(of: entries.map(\.id)) { ... }                // 1
.animation(.easeOut(...), value: entries.map(\.id))     // 2
.onChange(of: entries.map(\.id)) { ... }                // 3

Each SwiftUI body evaluation allocates three [UUID] arrays. Easy to eliminate by lifting to a let in the enclosing scope. Not a blocker.

3. selectedLayerOutput persists across picker opens

Previously @State on OverlayMapperSection, reset naturally with the view. Now it lives on MapperViewModel and is not cleared in systemActionPopover.onAppear. This is likely intentional (selection survives a reopen), but selectedLayerOutput can drift from viewModel.availableLayers if layers are added/removed between opens. The layerRow will just show a stale checkmark on a layer that no longer exists. Worth a comment or a defensive clear-if-missing in layersExpandedContent.

4. launchAppsExpandedContent mixes imperative bindings before return

var launchAppsExpandedContent: some View {
    let query = viewModel.launchAppSearchText.trimmingCharacters(...)
    let filteredApps = query.isEmpty ? knownApps : knownApps.filter { ... }
    return VStack(spacing: 0) { ... }
}

Valid Swift, but mixing imperative let + explicit return is slightly inconsistent with the surrounding some View property style. Could be inlined or extracted. Not a bug.

5. entries capture in dismiss closure

.onChange(of: entries.map(\.id)) { _, ids in
    if !ids.isEmpty {
        installDismissMonitors(dismissAll: { for entry in entries {
            entry.dismiss()
        } })
    }
}

entries is captured from the outer overlayPreferenceValue closure at onChange evaluation time, and the global monitor holds that dismissAll closure until the next id-set change triggers a reinstall. For the single-popover case here this is always fresh. A comment noting the assumption would help a future reader.

Test Coverage

No new unit tests — appropriate here. The bugs are in SwiftUI rendering pipeline behavior (preference key propagation, anchor computation) and AppKit first-responder flow, neither testable without a running window server. Manual verification on a signed/notarized build is the right gate.

Minor Nits

• @ViewBuilder on outputTypeLeafRow and outputTypeNavRow is redundant — both are single-expression functions returning a concrete type. Harmless.
• The // 53 = kVK_Escape comment survived the rename to installDismissMonitors. Still accurate, no action needed.

Summary

Approve with minor suggestions. The root cause identification is sharp, the fix is correct, and the drill-down refactor is a meaningful UX improvement that also eliminates the reflow/positioning fight that caused the old inline-expand rows to feel dead. The main items worth a follow-up comment (not blockers): note that AnyView(contentBuilder()) now runs per layout pass, and that selectedLayerOutput persists on the ViewModel across picker opens (stale-checkmark edge case if layers change between opens).

Code Review — Overlay Output-Type Picker Fix

Summary: Fixes an unresponsive output-type picker in the overlay mapper caused by two independent bugs, and replaces the inline-expand pattern with a fixed-frame drill-down.

Root Cause Fix — WindowAnchoredPopover.swift ✅

The diagnosis is correct and the fix is tight. Removing Equatable from WindowAnchoredPopoverEntry resolves the stale-content problem, and keying the ESC monitor and animation on entries.map(\.id) is the right way to preserve the "only rebind on open/close" intent without the broken equality. The comment block explaining why Equatable was removed is exactly the kind of WHY comment that belongs here.

One nuance worth noting: AnyView(contentBuilder()) now runs on every layout pass rather than being cached. For this picker tree the cost is negligible, but the removal of the cachedContent path makes future heavy content in a windowAnchoredPopover slightly riskier. A sentence in the windowAnchoredPopover doc comment warning callers about this trade-off would be useful if that surface gets reused.

FirstMouseHostingView ✅

Simple, correct. The @unchecked Sendable approach in this file is already established. The type parameter tying it to LiveKeyboardOverlayView is fine since it's single-site.

Global Mouse Monitor ⚠️ Minor concern

outsideClickMonitor = NSEvent.addGlobalMonitorForEvents(
    matching: [.leftMouseDown, .rightMouseDown, .otherMouseDown]
) { _ in
    dismissAll()
}

Global monitors are always installed/removed around the popover open lifecycle, which is correct. One edge case: the dismissAll closure captures entries from the enclosing onChange call site, but since installDismissMonitors is re-called whenever IDs change, the captured snapshot is always current — this is safe.

What's worth confirming: if the user opens a system dialog (e.g., NSOpenPanel for folder/script browse) while a popover is open, the panel's click will fire the global monitor and dismiss the popover before the panel even appears. That flow likely can't happen from the current picker (the leaf rows close the picker before opening the panel), but if browseForFolder / browseForScript are ever called with the picker still visible, this would cause a double-dismiss race. Not a bug today — just something to keep in mind.

AutoFocusTextField ✅

The didFocus guard on the coordinator is the right way to prevent focus-stealing on re-renders. The DispatchQueue.main.async deferral with a window != nil guard handles the "view not in window yet" case cleanly.

One minor note: there's no placeholder set on the NSTextField. The magnifying glass icon provides affordance, but if the field is ever reused in a context without that icon, the lack of a placeholder could be confusing. Consider exposing a placeholder: String = "" parameter even if unused by the current caller.

State Migration (selectedLayerOutput → MapperViewModel) ✅

The move is architecturally justified: the comment explains exactly why @State doesn't propagate from the hoisted layer. The outputPickerPage enum on MapperViewModel is clean and the cases map 1-to-1 to the real navigation graph.

Minor behavioral note: launchAppSearchText is only cleared in .onAppear of the full systemActionPopover (on picker open). If a user types a search, hits back, then re-enters Launch Apps within the same picker session, the prior search text reappears. This is probably not intended — consider also clearing launchAppSearchText inside the navigate-to-launchApps action:

outputTypeNavRow(...) {
    viewModel.launchAppSearchText = ""  // ← clear on each entry to sub-page
    loadKnownApps()
    viewModel.outputPickerPage = .launchApps
}

Drill-Down UX / Refactor ✅

outputTypeLeafRow and outputTypeNavRow are a clean factoring — the old 6× repeated button scaffolding is gone and the two patterns (leaf vs. nav) are clearly distinguished. The ZStack + .transition(.move(edge:)) approach produces correct iPhone-style directional animation: forward = trailing entry, backward = leading return.

The fixed 280×340 frame is the right call here given the hoisted-layer reflow issue it solves. The ScrollView inside each sub-page handles overflow without needing dynamic height.

Test Coverage

No new unit tests, but the changed behavior is deep in AppKit/SwiftUI integration territory (acceptsFirstMouse, preference keys, global monitors) that doesn't lend itself to XCTest. The manual verification on a signed/notarized build is appropriate given the constraint. If OutputPickerPage state transitions grow more complex in the future, a unit test on MapperViewModel for the navigation state would be easy to add.

Checklist

• Root bug correctly diagnosed and fixed
• Second bug (first-click) addressed with FirstMouseHostingView
• State migration justified by architectural constraint
• collapseAllSections() cleanly eliminated
• Global monitor lifecycle (install/remove) is correct
• Consider clearing launchAppSearchText on sub-page entry (minor)
• Consider placeholder parameter on AutoFocusTextField for reusability (minor)

Overall this is solid work — the root cause analysis is thorough, the fix is targeted, and the drill-down refactor is significantly cleaner than the inline-expand pattern it replaces. The two minor items above are suggestions, not blockers.